Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support Call and Return components in React.Children calls #11422

Merged
merged 6 commits into from
Nov 20, 2017

Conversation

MatteoVH
Copy link

@MatteoVH MatteoVH commented Nov 1, 2017

Solves #11416

@gaearon, I tried to read up a bit on Call and Return, but I'm (obviously seeing as it's an experimental API) inexperienced with them. Let me know if I'm testing their usage satisfactorily!

@@ -28,6 +28,12 @@ var REACT_ELEMENT_TYPE =
const REACT_PORTAL_TYPE =
(typeof Symbol === 'function' && Symbol.for && Symbol.for('react.portal')) ||
0xeaca;
const REACT_CALL_TYPE =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: let's reorder them before the portal (the numbers go in sequential order).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

@@ -23,6 +23,20 @@ describe('ReactChildren', () => {
ReactTestUtils = require('react-dom/test-utils');
});

const checkReactChildrenFunctionality = (element, callback, context) => {
const parentInstance = <div>{element}</div>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just copy and paste this part between tests. It's fine for tests to be verbose. That way they're easier to debug.

@MatteoVH
Copy link
Author

MatteoVH commented Nov 1, 2017

Ready for re-review!

@@ -128,6 +134,8 @@ function traverseAllChildrenImpl(
// The following is inlined from ReactElement. This means we can optimize
// some checks. React Fiber also inlines this logic for similar purposes.
(type === 'object' && children.$$typeof === REACT_ELEMENT_TYPE) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we return this into a switch somehow?

0xeac8;
const REACT_RETURN_TYPE =
(typeof Symbol === 'function' && Symbol.for && Symbol.for('react.return')) ||
0xeac9;
const REACT_PORTAL_TYPE =
(typeof Symbol === 'function' && Symbol.for && Symbol.for('react.portal')) ||
Copy link
Contributor

@nhunzaker nhunzaker Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to share the typeof Symbol === 'function' && Symbol.for here? in a supportsSymbolFor variable, or something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter right now. This code executes once. Although it probably makes sense to copy how we do it in ReactChildFiber.

@gaearon
Copy link
Collaborator

gaearon commented Nov 6, 2017

Friendly ping :-)

@MatteoVH
Copy link
Author

MatteoVH commented Nov 8, 2017

Will have time tonight to re-factor!

@MatteoVH MatteoVH force-pushed the react-children-call-return-support branch from ba323c9 to 7eb6bba Compare November 10, 2017 20:04
@MatteoVH
Copy link
Author

Sorry for the late response, I tried to move some of the if logic into a switch, and it just didn't seem to cleanly fit into that construct. I left one attempt, but I think it's more legible as it was before @gaearon.

@MatteoVH MatteoVH force-pushed the react-children-call-return-support branch from 7eb6bba to 3d22e79 Compare November 10, 2017 20:16
(type === 'object' && children.$$typeof === REACT_ELEMENT_TYPE) ||
(type === 'object' && children.$$typeof === REACT_PORTAL_TYPE)
) {
const invokeCallback = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very hot path. We don't want to create closures here. It's fine to duplicate code instead if necessary.

@gaearon
Copy link
Collaborator

gaearon commented Nov 20, 2017

Instead of duplicating, can we just set a boolean flag? It starts out being false. Under conditions, we flip it to true. Then we check it at the bottom, and fire the callback if it is true.

@gaearon gaearon merged commit c6bde7b into facebook:master Nov 20, 2017
@gaearon
Copy link
Collaborator

gaearon commented Nov 20, 2017

Nice. Thanks.

Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
…11422)

* support Call and Return components in React.Children calls

* make tests more verbose

* fix ordering of React component types

* cleanup conditional detection of children type

* directly inline callback invocation

* reduce callback invocation code re-use
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants